Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't close the audio reader which might be linked from a shallow copy. #1059

Closed
wants to merge 8 commits into from
Closed

Don't close the audio reader which might be linked from a shallow copy. #1059

wants to merge 8 commits into from

Conversation

jwg4
Copy link
Contributor

@jwg4 jwg4 commented Jan 16, 2020 •
edited
Loading

This fixes #1025

A recent change closed the audio reader object when the video file reader object was closed. However the former could be shared between the clip being closed and another clip which was still needed, because of shallow copying, I think it's fine to let the garbage collector del the reader object when it's definitively no longer needed.

(Strikethrough - not applicable or not needed):

  • If this is a bugfix, I have provided code that clearly demonstrates the problem and that works when used with this PR
  • I have added a test to the test suite, if necessary
  • I have properly documented new or changed features in the documention, or the docstrings
  • I have properly documented unusual changes to the code in the comments around it
  • I have made note of any breaking/backwards incompatible changes

@coveralls
Copy link

coveralls commented Jan 16, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 64.99% when pulling 08d20c0 on jwg4:patch-2 into d54c195 on Zulko:v1.0.x.

@jwg4 jwg4 changed the title Add a test which checks for killed audio reader. Don't close the audio reader which might be linked from a shallow copy. Jan 29, 2020
@jwg4
Copy link
Contributor Author

jwg4 commented Jan 29, 2020

The appveyor build is broken - not just on my branch. I have fixed this in #1067

jwg4 and others added 4 commits January 29, 2020 01:16
@jwg4
Add a test which checks for killed audio reader.
a06402a
@jwg4
Don't explicitly close the proc in the reader.
18b36bd 
Don't shut down the reader - it could be shared between multiiple clip instances. Just unset it here and the GC should kill it when it's no longer referenced anywhere.
@jwg4
Change the frame we look for.
8670b4b
@jwg4
We need a longer clip for the test to work correctly.
78e4614
@tburrows13 tburrows13 added the bug-fix For PRs and issues solving bugs. label Mar 10, 2020
@tburrows13 tburrows13 mentioned this pull request Apr 24, 2020
Open
7 tasks
@tburrows13 tburrows13 changed the base branch from master to v1.0.x April 30, 2020 23:45
@tburrows13
Copy link
Collaborator

tburrows13 commented May 1, 2020 •
edited
Loading

I've confirmed that this fixes #938 which has been a major bug for the past year on both macOS and Windows (and presumably linux). I wanted to properly investigate the handling and closing of external processes in v2.0, but that is likely still some time away so I'd like to release a v1.0.3 with just this PR in. The only catch is that with this PR, Windows often fails with OSError: [WinError 6] The handle is invalid in Python 3.6 and below due to https://bugs.python.org/issue37380 (See also #1066), so basically we would only be able to support Py3.7+ on windows. I hope that a proper fix can be found for v2.0 that would re-enable support for Py3.6, but that will have to come later.

I don't have any time to investigate further anytime soon, so if we don't release this PR, then we'll presumably have to hold off until v2.0.
I think that this trade off is worth it, and it can be announced in the patch notes for v1.0.3.

@Zulko (or anyone else) please advise.

You can compare the 2 latests commits (ignore their names) to see the difference that this creates.

This was referenced May 7, 2020
Closed
Merged
@tburrows13
Copy link
Collaborator

Thank you for the contribution! The test in particular was very helpful when trying to diagnose the issue. It has been included in #1185, which has superseded this PR, so I am closing this one. I'd be happy to receive PRs about further improvements to how clips are deleted and closed, but they would have to be fully thought out and work the same with both video and audio.

@tburrows13 tburrows13 closed this May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug-fix For PRs and issues solving bugs.
Projects
Investigation into resource errors an...
Pull requests
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AttributeError: 'NoneType' object has no attribute 'get_frame'
3 participants
@jwg4 @coveralls @tburrows13